Conversation
Introduces .pre-commit-config.yaml to automate code formatting, linting, and basic checks using pre-commit hooks for improved code quality and consistency.
Upgraded GitHub Actions to use newer versions and improved test steps by separating model benchmark and unit tests, adding coverage reporting with codecov. Added pytest-cov to dev dependencies and configured Ruff linter settings in pyproject.toml.
Introduces detailed unit tests for the Assembler, Assembly, Joint, and Link classes in dlclive.core.inferenceutils. The new tests cover metadata parsing, detection flattening, link extraction, assembly building, Mahalanobis distance calculation, I/O helpers, and various Assembly operations, improving test coverage and reliability.
Corrects color sampling in Display to avoid zero step and ensures image is always defined in display_frame. Adds comprehensive tests for Display, including headless environment setup, frame display, cutoff logic, window destruction, and color sampling safety.
Removed the --no-cache flag from 'uv sync' in the testing workflow, enhanced pytest coverage reporting, and added a step to summarize coverage in the GitHub Actions job summary. Added a note in dynamic_cropping.py about duplication with another file and referenced existing tests.
Introduces a new 'pose' attribute to the DLCLive class, initialized as None or a numpy ndarray. This prepares the class for storing pose data.
sneakers-the-rat
left a comment
sneakers-the-rat
left a comment
There was a problem hiding this comment.
Love it love it. We love tests. We love linting. I dont know too much about the code under test but gave a quick skim
Move test helpers into tests/conftest.py and introduce a suite of reusable pytest fixtures for assembler testing (headless_display_env, assembler graph/paf fixtures, scene factories, assembler/assembly/joint/link factories, and various canned dataset variants). Refactor tests/tests_core/test_assembler.py and tests/tests_core/test_assembly.py to consume the new fixtures, remove duplicated setup code, and simplify assertions. Also adjust serialization test filenames and tidy up identity/affinity-related test logic.
Replace handcrafted FakeTk/Label/PhotoImage classes with unittest.mock MagicMocks in the headless_display_env fixture. The fixture now returns a SimpleNamespace with mock constructors/instances (tk, label, photo) and sets display module attributes with monkeypatch (raising=False). Update tests to use the env mocks, assert on mock calls (title, pack, configure, update, destroy), mock ImageDraw.Draw with a MagicMock, and make colormap sampling deterministic for assertions. This simplifies test setup and enables precise call-based assertions instead of inspecting custom fake object state.
Replace tuple-based assembler fixtures with SimpleNamespace objects (providing .data, .graph, .paf_inds) and update all tests to access those attributes. Add Hypothesis to dev dependencies and introduce property-based tests for Assembly (from_array invariants and extent/area checks)
Introduce property-based tests for assembler utilities using Hypothesis: add test_condensed_index_properties for _conv_square_to_condensed_indices, a composite coords_and_conf strategy, test_flatten_detections_counts to validate _flatten_detections output counts, and a property test for extract_best_links (greedy) that asserts affinity, confidence-product (pcutoff) and disjointness invariants. Add Hypothesis imports, settings and numpy-array strategies. Also adjust test_assembly to ensure rows containing any NaN are set fully to NaN so the test matches Assembly.from_array behavior.
|
Thanks again @sneakers-the-rat for the feedback, I've tried to integrate it best as I could. I also checked out other tools to help with robustness such as |
There was a problem hiding this comment.
Pull request overview
This PR expands the testing suite with comprehensive tests for core modules (Assembly, Assembler, Display) using Hypothesis for property-based testing, adds minor code quality improvements, and flags code duplication with the main DeepLabCut codebase.
Changes:
- Comprehensive test coverage for
AssemblyandAssemblerclasses with hypothesis-based testing - New test fixtures in
conftest.pyfor assembly/assembler testing scenarios - Code quality improvements: modernized type hints, simplified expressions, added
stacklevelto warnings
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/tests_core/test_assembly.py | Comprehensive tests for Assembly class with property-based testing |
| tests/tests_core/test_assembler.py | Comprehensive tests for Assembler class with property-based testing |
| tests/test_display.py | Tests for Display class using mocked tkinter |
| tests/conftest.py | Test fixtures for assembly/assembler/display testing |
| tests/test_modelzoo.py | Updated duplication comment format |
| tests/test_benchmark_script.py | Added formatting and slow marker |
| pytest.ini | Added slow test marker |
| pyproject.toml | Added pytest-cov and hypothesis dependencies |
| dlclive/pose_estimation_pytorch/dynamic_cropping.py | Simplified error messages, removed unused import |
| dlclive/modelzoo/utils.py | Formatting improvements, updated duplication comments |
| dlclive/modelzoo/resolve_config.py | Simplified error messages, updated duplication comments |
| dlclive/dlclive.py | Minor formatting and added pose attribute |
| dlclive/display.py | Fixed potential zero-step bug in color sampling |
| dlclive/core/inferenceutils.py | Modernized type hints, added duplication note, minor refactoring |
| .pre-commit-config.yaml | Added pre-commit hooks configuration |
| .github/workflows/testing.yml | Updated test job names, commented out coverage reporting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deruyter92
left a comment
deruyter92
left a comment
There was a problem hiding this comment.
Great PR, thanks for adding so much test functionality! That's really needed.
I paid extra attention to the changes to display and the added testing. I left a comment about the clamping (and the testing thereof). For the rest, looks all good to me!
(and I like the use of hypothesis for testing, will look into it.)
Corrects swapped im_size indexing when clamping ellipse coordinates in Display.draw. The previous code used im_size[1] for x1 and im_size[0] for y1, which reversed width/height fallbacks and could produce incorrect bounds; x1 now uses im_size[0] (width) and y1 uses im_size[1] (height). This prevents drawing outside the image or using wrong coordinates.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace verbose conditional expressions with max/min to clamp pose-based ellipse coordinates (x0, x1, y0, y1) within image bounds.
3 participants
Expanded testing suite, addressing some missing coverage. Adds coverage reporting to the CI.
Focuses on core functionalities testing, and avoids redundancy with main DLC codebase (but pends further centralization)
hypothesis-based testing to improve utility and robustness of testsMinor fixes of some missing attributes.
Flags code duplication from main DLC codebase (may not be exhaustive yet)
Remove CI changes for now
NOTE : Contains some CI/pre-commit changes, these can be removed before targeting main if we want them in a separate PR (but they will be used for formatting)This pull request includes several improvements and updates across the codebase and CI configuration. The main themes are: adding pre-commit hooks for code quality, updating the test workflow and coverage reporting, and refactoring the
dlclive/core/inferenceutils.pyfile for code clarity and modern Python standards.Continuous Integration and Code Quality:
.pre-commit-config.yamlfile to configure pre-commit hooks for docstring checks, whitespace cleanup, setup.cfg formatting, and Ruff linter/formatter..github/workflows/testing.ymlto use newer action versions, streamline dependency installation, and add code coverage reporting with Codecov. [1] [2] [3]Refactoring and Modernization of
dlclive/core/inferenceutils.py:tuple[float, float]instead ofTuple[float, float],zip(..., strict=False)for safer iteration). [1] [2] [3] [4] [5] [6] [7]stacklevelparameter for better tracebacks and improved readability of multi-line code blocks. [1] [2] [3]Documentation and Comments:
dlclive/core/inferenceutils.pyindicating that the file is a duplicate of the original DeepLabCut codebase, with author and date.